Role-binding flag text, linter bool-flag exception, logout no-op. - #3423
Open
David Adams (davidadas) wants to merge 7 commits into
Open
Role-binding flag text, linter bool-flag exception, logout no-op.#3423David Adams (davidadas) wants to merge 7 commits into
David Adams (davidadas) wants to merge 7 commits into
Conversation
…ng list
The flag's help text said "If scopes are unspecified, list only
organization-scoped role bindings," but the command's own documented
example ("for all scopes") and actual behavior return role bindings
across all scopes (org, environment, cluster) in that case. Corrected
the description to match, and updated the 4 golden fixtures that
pinned the old text.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…g the linter RequireValidExamples() flagged any --flag=value in an example, but boolean flags legitimately need "=" to set the non-default value (e.g. --flag=false). Added getBoolFlags() and excluded them from the "=" check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
logout used NewAuthenticatedCLICommand, which required being logged in (erroring "not logged in" with no context) and would auto-login via env-var credentials only to immediately log back out. Switched to NewAnonymousCLICommand (same pattern as `login`) and return immediately when there's no active context, instead of erroring or auto-authenticating. The ccloud client used to revoke the refresh token is now constructed directly in revokeCCloudRefreshToken rather than relying on the Authenticated PreRun to populate it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
--inclusive is only read by the Cloud-path functions (listMyRoleBindings, ccloudListRolePrincipals), both called exclusively from ccloudList. The on-prem path (confluentList) never reads it, even though the flag is registered unconditionally for both login modes. Clarified the flag description as Confluent Cloud-only so on-prem users don't think it does something. Updated all 4 golden fixtures that pin this text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
revokeCCloudRefreshToken accessed c.Version.UserAgent directly, but some PreRunner mocks (e.g. internal/login's cross-package test via mock.Commander) never populate Version, causing a nil pointer panic. Confirmed via the full test suite: TestLoginWithExistingContext panicked in internal/login before this fix, passes after it. Guard with a nil check instead of relying on every PreRunner implementation to set Version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
13 tasks
There was a problem hiding this comment.
Pull request overview
Fixes three user-facing and developer-tooling issues in the Confluent CLI: clarifies --inclusive help text for role-binding listing, corrects the linter’s handling of boolean --flag=value usage in examples, and updates confluent logout to be a true no-op when already logged out (avoiding auto-login/logout via env credentials).
Changes:
- Update
iam rbac role-binding list --inclusiveflag help text and refresh affected golden fixtures. - Relax
RequireValidExamples()lint rule to allow--bool-flag=valuesyntax in examples. - Switch
logoutto an anonymous pre-run and return early with no output when no context is active.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/fixtures/output/iam/rbac/role-binding/list-help.golden | Updates expected cloud help output for revised --inclusive description. |
| test/fixtures/output/iam/rbac/role-binding/list-help-onprem.golden | Updates expected on-prem help output for revised --inclusive description. |
| test/fixtures/output/iam/rbac/role-binding/list-failure-help-onprem.golden | Updates expected on-prem failure help output for revised --inclusive description. |
| test/fixtures/output/iam/rbac/role-binding/list-failure-help-cloud.golden | Updates expected cloud failure help output for revised --inclusive description. |
| pkg/linter/command_rules.go | Excludes boolean flags from the “no = in examples” lint rule via getBoolFlags(). |
| internal/logout/command.go | Makes logout runnable while logged out (no-op) and constructs the CCloud client directly for token revocation. |
| internal/iam/command_rbac_role_binding_list.go | Updates --inclusive flag help string. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Both the logout no-op branch and the new getBoolFlags/RequireValidExamples "=" exception had zero test coverage, and RequireValidExamples' cognitive complexity (23) exceeded the allowed threshold (15) once the boolFlags check was added. Add direct unit tests for the no-op logout path and the bool-flag example exception, and split RequireValidExamples' three validation loops into named helper functions to bring complexity back under the limit.
- Guard against a missing/corrupt context_state entry in revokeCCloudRefreshToken, which previously panicked on a nil ContextState instead of letting logout succeed as a no-op revoke. - Add an integration test covering `confluent logout` run a second time with no active session, locking in the APIE-1314 no-op behavior end-to-end (not just at the unit level).
|
David Adams (davidadas)
marked this pull request as ready for review
July 31, 2026 13:01
Steven Gagniere (sgagniere)
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release Notes
Bug Fixes
confluent iam rbac role-binding list --inclusivehad a help-text description that contradicted its own documented example and actual behavior. (APIE-1362)RequireValidExamples()lint rule incorrectly flagged boolean flags using--flag=valuesyntax in command examples. (APIE-1286)confluent logouterrored or silently auto-logged-in-then-out instead of being a no-op when already logged out. (APIE-1314)Checklist
Whatsection below whether this PR applies to Confluent Cloud, Confluent Platform, or both.Test & Reviewsection below.Blast Radiussection below.Unchecked items need human follow-up before merge — no Go toolchain was available in the environment this PR was prepared in, so no build/lint/test run or live verification was possible. This is more important than usual for this PR since APIE-1314 changes which PreRun a command uses — please run
make build && make lint && make test(especiallyTestLogout_RemoveUsernamePasswordandTestLogout_RemoveUsernamePasswordFail) before merging. Opening as a draft for that reason.What
Confluent Cloud and Confluent Platform (on-prem) both — all three fixes touch shared code paths used by both login modes.
--inclusiveflag's help text said "If scopes are unspecified, list only organization-scoped role bindings", contradicting the command's own documented example ("for all scopes") and the reporter's confirmed actual behavior. Corrected the text and the 4 golden fixtures that pinned the old wording.RequireValidExamples()(pkg/linter/command_rules.go) flagged any--flag=valuein an example. Boolean flags legitimately need=to set the non-default value (e.g.--flag=false). Added agetBoolFlags()helper and excluded boolean flags from that check.confluent logoutusedNewAuthenticatedCLICommand, which requires being logged in. With no active session this producedError: not logged in; withCONFLUENT_CLOUD_API_KEY/SECRETenv vars set, it auto-logged-in only to immediately log back out. Switched toNewAnonymousCLICommand(the same patternloginitself uses) and return immediately with no error when there's no active context. The ccloud client used to revoke the refresh token (previously populated by theAuthenticatedPreRun) is now constructed directly insiderevokeCCloudRefreshToken, only when a real session is confirmed to exist.Blast Radius
--helpdescription string. No behavior or serialized-output change.make lint-cli); no runtime CLI behavior changes for customers.confluent logout. Customers who are genuinely logged in see no change (still revokes the token, persists logout, prints the same success message). Customers who runlogoutwhile already logged out previously saw an error or an auto-login/logout cycle; they'll now see nothing happen at all, matching the ticket's explicit ask. Worth a careful look during review since it changes which PreRun the command uses.References
Test & Review
No Go toolchain was available in the environment this PR was prepared in, so
make build/make lint/make testwere not run here — please run these before merging, and pay particular attention totest/logout_test.gogiven the APIE-1314 change.What was actually verified:
RequireValidExamples(), so nothing to break; the newgetBoolFlags()helper mirrors the existinggetAllFlags()/getRequiredFlags()pattern in the same file.NewAnonymousCLICommandis the same constructor already used byinternal/login/command.go; tracedcommand.Version = r.Versioninto theAnonymous()PreRun function (pkg/cmd/prerunner.go:108) to confirmc.Versionis populated beforerevokeCCloudRefreshTokenneeds it; confirmed the existingtest/logout_test.gotests always log in immediately before logging out, so the no-op path isn't exercised by them (a new test for the no-op case would be worth adding).